-
-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add irace #227
Conversation
irace::readParameters(text = par.tab) | ||
} | ||
getIraceRange = function(ps){ | ||
rng = data.table(lower = ps$lower, upper = ps$upper, lvl = ps$levels) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as.data.table()
should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can probably do something with
as.data.table(ps)[, ifelse(is.na(lower),
<whatever paste collapse 'levels'>,
paste0("(", lower, ",", upper, ")"))]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know the best way to collapse lists with some elements characters and some not?
as.data.table(ps)[,ifelse(is.na(lower),
paste0("(",paste0(levels, collapse = ","),")"),
paste0("(",lower,",",upper,")"))]
Doesn't work as it collapses all levels
and ignores the `ifelse, e.g. I got:
"c(TRUE, FALSE),NULL,c(\"lvl1\", \"lvl2\"),NULL" "(1,9)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add by = id
and use levels[[1]]
:
as.data.table(ps(x = p_dbl(1, 4), y = p_lgl()))[,
ifelse(is.na(lower),
paste0("(",paste0(levels[[1]], collapse = ","),")"),
paste0("(",lower,",",upper,")")),
by = "id"]$V1
#> [1] "(1,4)" "(TRUE,FALSE)"
some small comments for now:
|
|
`targetRunner` kept as-is to be in line with `irace` convention
Codecov Report
@@ Coverage Diff @@
## main #227 +/- ##
=======================================
Coverage ? 92.30%
=======================================
Files ? 20
Lines ? 390
Branches ? 0
=======================================
Hits ? 360
Misses ? 30
Partials ? 0 Continue to review full report at Codecov.
|
Done, done, done.
Done - Need to check how dependencies are working though. I'm unsure if some tests working as expected. Also added a few dependencies to suggests that could be removed if needed by changing tested learners |
Shall we try to finish, merge and close this PR? @mb706 @berndbischl . Open questions seem to be about correctly setting |
We should first merge #240 and then I will probably have to adapt this PR to the API changes. |
okay great, I've converted to a draft for now |
Have you merged the master into your branch, check that tests are complete and running, mark this PR as ready for review and we will merge it if everyhting is fine. |
Okay so pinging everyone who replied above many months ago: @jakob-r @be-marc @mb706 I've merged in Master and got it working, however there seems to be a problem upstream causing warnings ('partial match') you can see it on all the builds. For most of the tests I can handle this with About the tuner itself, some things I'd appreciate double checking before merging, sorry I don't know the internals of mlr3tuning or irace well so a lot of my implementation is unfortunately guesswork:
Thanks for the help with this EDIT: For some reason the warnings didn't show up in the tests this time, so it's possibly stochastic? Which seems weird given the nature of the warnings in the previous tests, either way looks good now. |
These checks are enabled by our You can temporarily disable them if required. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort. Looks good for the most part. Have we discussed it this would be worth an extra package?
Does it fail bc. irace is taking a tiny bit longer than the terminator is allowing? |
Our documentation stated
I added
|
It does not fail often. We substract 5 seconds now. Maybe it helps. |
Co-authored-by: Jakob Richter <[email protected]>
Closes #63 |
warnPartialMatchAttr = TRUE, | ||
warnPartialMatchDollar = TRUE | ||
) | ||
# The upstream package causes partial match warnings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
irace
uses rep
with length
instead of length.out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reported upstream MLopez-Ibanez/irace#7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what is worth, this issue is now fixed upstream so you should be able to re-enable the warnings. We, the irace team (which is basically 2 people doing both teaching and research), are happy to receive issues and pull requests for fixes like these but also for changes that simplify integration with mlr (or other packages).
I think this can be merged. Any objections? |
irace_helpers.R
for helper functions to makeTunerIrace
a bit neaterTunerIrace
Also changed the template very slightly to include all three constructor methods. If you dislike this let me know and I'll revert back.